Enable appsembler reporting settings - #177
Conversation
Added capability to define settings.APPSEMBLER_REPORTING dict if APPSEMBLER_FEATURES has 'ENABLE_APPSEMBLER_REPORTING' to true
|
|
||
|
|
||
| if APPSEMBLER_FEATURES.get('ENABLE_APPSEMBLER_REPORTING', False): | ||
| from appsembler_reporting.settings import APPSEMBLER_REPORTING |
There was a problem hiding this comment.
Let me know if you think I need to wrap this in a try block. I didn't because this is only called if lms.env.json has ENABLE_APPSEMBLER_REPORTING as not False
There was a problem hiding this comment.
@johnbaldwin I'm not sure what you mean by
Note: This will fail if server-vars/lms.env.json has 'APPSEMBLER_FEATURES' 'ENABLE_APPSEMBLER_REPORTING as true. BUT this setting is not enabled for existing deployments that use appsembler/ficus/develop or appsembler/ficus/open-university/*
Can you rephrase or clarify? Why would it fail? Isn't the point to have this work when ENABLE_APPSEMBLER_REPORTING is true?
There was a problem hiding this comment.
I'd probably include a try block, for cases where ENABLE_APPSEMBLER_REPORTING is accidentally left set true but the package is missing.
There was a problem hiding this comment.
@bryanlandia if the appsembler_reporting is not installed then the import will fail, but to make this happen you'd have to set:
"APPSEMBLER_FEATURES": {
"ENABLE_APPSEMBLER_REPORTING": true
}
AND not pip install appsembler_reporting
Does that help explain?
There was a problem hiding this comment.
A little bit belt and suspenders, but I'll add a try block and log an error message, so there is at least a clue that something is not right if the installer enables reporting but forgets to add the package
There was a problem hiding this comment.
@bryanlandia Before I add a logger to lms/envs/appsembler.py, how would you suggest logging errors that might happen in envs/appsembler.py. Thing is, I don't want this to silently fail since the only way this can fali now is if the site is misconfigured
There was a problem hiding this comment.
I would just think to do a basic
import logging
logger = logging.getLogger(__name__)
try:
...
except ImportError:
logger.warn('FOO')
so it shows up in the main /edx/var/log/lms/edx.log
There was a problem hiding this comment.
So I'm less inclined to wrap this import in a try block considering the options wen the site is misconfigured (meaning appsembler_reporting NOT installed and ENABLE_APPSEMBLER_REPORTING is true:
- Site gets a 500 and an ImportError is shown in the logs: Should be clear what to fix there
- Silently fail in running reporting and unless the developer knows what shakes, spend some cycles debuging
- Figure out how we want to alert the admin that the site is misconfigured in production mode
Or am I missing something fundamental?
There was a problem hiding this comment.
I was just thinking more about it... yes, you're right.
There was a problem hiding this comment.
cool, thanks! One thing I'm thinking about as we've talked over this is some easy way to get an "App health report" when we start or restart the LMS/CMS That eschews noise and just logs any failures
|
@bryanlandia Thanks, super appreciate the review! |
Added capability to define settings.APPSEMBLER_REPORTING dict if
APPSEMBLER_FEATURES has 'ENABLE_APPSEMBLER_REPORTING' to true
For the
ficus-common-codebase, I'm going to push an update to the appsembler_reporting repo to have the settings.py file andAPPSEMBLER_REPORTINGdictNote: This will fail if server-vars/lms.env.json has 'APPSEMBLER_FEATURES' 'ENABLE_APPSEMBLER_REPORTING as true. BUT this setting is not enabled for existing deployments that use
appsembler/ficus/developorappsembler/ficus/open-university/*Purpose is to have as small change as possible in edx-platform to support appsembler reporting development and customization